feat(solana): deposit command #317
Conversation
|
Warning Rate limit exceeded@fadeev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 41 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe updates include raising the Node.js version from 18 to 21 in all GitHub Actions workflows, upgrading the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI (depositCommand)
participant KeypairHelper
participant SolanaProvider
participant GatewayProgram
User->>CLI (depositCommand): Run deposit command with options
CLI (depositCommand)->>KeypairHelper: Derive Keypair (from mnemonic or private key)
KeypairHelper-->>CLI (depositCommand): Return Keypair
CLI (depositCommand)->>SolanaProvider: Connect to selected network
SolanaProvider-->>CLI (depositCommand): Return provider
CLI (depositCommand)->>GatewayProgram: Initialize with IDL and provider
CLI (depositCommand)->>GatewayProgram: Call deposit/depositSplToken with params
GatewayProgram-->>CLI (depositCommand): Return transaction hash
CLI (depositCommand)->>User: Output transaction hash
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/commands/src/solana/deposit.ts (2)
124-124: Purpose of null parameter is unclear.Both
depositSplTokenanddepositmethods have anullparameter, but there's no comment explaining what this parameter is for.Add comments to explain the purpose of this parameter:
- null + null // Message parameter - null means no additional message dataAlso applies to: 141-141
161-163: Consider adding a default network.The network option doesn't have a default value, which means it must be explicitly provided. Consider setting a default, such as "devnet" for ease of use.
.addOption( - new Option("--network <network>", "Solana network").choices(networks) + new Option("--network <network>", "Solana network").choices(networks).default("devnet") )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (29)
typechain-types/@uniswap/index.tsis excluded by!typechain-types/**typechain-types/@uniswap/v3-core/contracts/index.tsis excluded by!typechain-types/**typechain-types/@uniswap/v3-core/contracts/interfaces/callback/IUniswapV3SwapCallback.tsis excluded by!typechain-types/**typechain-types/@uniswap/v3-core/contracts/interfaces/callback/index.tsis excluded by!typechain-types/**typechain-types/@uniswap/v3-core/contracts/interfaces/index.tsis excluded by!typechain-types/**typechain-types/@uniswap/v3-core/index.tsis excluded by!typechain-types/**typechain-types/@uniswap/v3-periphery/contracts/index.tsis excluded by!typechain-types/**typechain-types/@uniswap/v3-periphery/contracts/interfaces/ISwapRouter.tsis excluded by!typechain-types/**typechain-types/@uniswap/v3-periphery/contracts/interfaces/index.tsis excluded by!typechain-types/**typechain-types/@uniswap/v3-periphery/index.tsis excluded by!typechain-types/**typechain-types/contracts/SwapHelpers.sol/SwapLibrary.tsis excluded by!typechain-types/**typechain-types/contracts/SwapHelpers.sol/index.tsis excluded by!typechain-types/**typechain-types/contracts/index.tsis excluded by!typechain-types/**typechain-types/factories/@uniswap/index.tsis excluded by!typechain-types/**typechain-types/factories/@uniswap/v3-core/contracts/index.tsis excluded by!typechain-types/**typechain-types/factories/@uniswap/v3-core/contracts/interfaces/callback/IUniswapV3SwapCallback__factory.tsis excluded by!typechain-types/**typechain-types/factories/@uniswap/v3-core/contracts/interfaces/callback/index.tsis excluded by!typechain-types/**typechain-types/factories/@uniswap/v3-core/contracts/interfaces/index.tsis excluded by!typechain-types/**typechain-types/factories/@uniswap/v3-core/index.tsis excluded by!typechain-types/**typechain-types/factories/@uniswap/v3-periphery/contracts/index.tsis excluded by!typechain-types/**typechain-types/factories/@uniswap/v3-periphery/contracts/interfaces/ISwapRouter__factory.tsis excluded by!typechain-types/**typechain-types/factories/@uniswap/v3-periphery/contracts/interfaces/index.tsis excluded by!typechain-types/**typechain-types/factories/@uniswap/v3-periphery/index.tsis excluded by!typechain-types/**typechain-types/factories/contracts/SwapHelpers.sol/SwapLibrary__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/SwapHelpers.sol/index.tsis excluded by!typechain-types/**typechain-types/factories/contracts/index.tsis excluded by!typechain-types/**typechain-types/hardhat.d.tsis excluded by!typechain-types/**typechain-types/index.tsis excluded by!typechain-types/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
.github/workflows/build.yaml(1 hunks).github/workflows/lint.yaml(1 hunks).github/workflows/test.yaml(1 hunks)package.json(2 hunks)packages/client/src/solanaDeposit.ts(1 hunks)packages/client/src/solanaDepositAndCall.ts(1 hunks)packages/commands/src/solana/deposit.ts(1 hunks)packages/commands/src/solana/index.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/commands/src/solana/deposit.ts (1)
packages/tasks/src/account.ts (1)
main(121-169)
🔇 Additional comments (14)
package.json (1)
121-121: Dependency bump for @zetachain/protocol-contracts-solana
Updated the Solana protocol-contracts dependency from2.0.0-rc1to3.0.2-rc2. Ensure compatibility with the new Gateway IDL and that theyarn.lockhas been updated and committed to avoid version mismatches..github/workflows/lint.yaml (1)
18-18: Upgrade Node.js runtime to 21
Aligns the lint workflow environment with build and test pipelines.actions/setup-node@v3supports Node.js 21 out of the box..github/workflows/build.yaml (1)
18-18: Upgrade Node.js runtime to 21
Consistent Node.js version across CI ensures uniform behavior. No additional changes needed..github/workflows/test.yaml (1)
18-18: Upgrade Node.js runtime to 21
Brings the test workflow in line with build and lint jobs—good for consistency.packages/client/src/solanaDeposit.ts (1)
8-8:✅ Verification successful
Switch to development Gateway IDL path
The import was changed to@zetachain/protocol-contracts-solana/dev/idl/gateway.json. Confirm that this path exists in version3.0.2-rc2of the package and that any reference insolanaDepositAndCall.tsmatches.To verify, you can run:
🏁 Script executed:
#!/usr/bin/env bash # Verify that the development IDL exists in the installed package if [ ! -f "node_modules/@zetachain/protocol-contracts-solana/dev/idl/gateway.json" ]; then echo "ERROR: dev/idl/gateway.json not found in protocol-contracts-solana" exit 1 else echo "OK: Gateway IDL file found" fiLength of output: 150
🏁 Script executed:
#!/usr/bin/env bash # Verify that solanaDepositAndCall.ts uses the same development IDL path rg -n 'from\s+".*@zetachain/protocol-contracts-solana/dev/idl/gateway.json"' packages/client/src/solanaDepositAndCall.ts || \ echo "WARNING: No matching import of gateway.json found in solanaDepositAndCall.ts"Length of output: 209
Development Gateway IDL path verified
The import path@zetachain/protocol-contracts-solana/dev/idl/gateway.jsonexists in version 3.0.2-rc2 and is consistently used in both files; no changes required.• packages/client/src/solanaDeposit.ts — line 8
• packages/client/src/solanaDepositAndCall.ts — line 8packages/client/src/solanaDepositAndCall.ts (1)
8-8: Updated Gateway IDL import path to use development version.The import path for the Gateway IDL has been updated to use the development version from the
/devdirectory. This change aligns with the dependency upgrade to@zetachain/[email protected]and supports the new deposit functionality.packages/commands/src/solana/index.ts (1)
3-3: New deposit command successfully integrated.The deposit command import and registration look good. This properly exposes the new Solana deposit functionality to the CLI.
Also applies to: 11-11
packages/commands/src/solana/deposit.ts (7)
1-15: Import statements look good.All necessary dependencies are imported for Solana interaction, token handling, and CLI commands. Both development and production Gateway IDLs are properly imported.
18-28: DepositOptions interface is well-defined.The interface provides all necessary parameters for deposit operations, supporting both native SOL and SPL token deposits.
30-47: Key derivation functions implemented correctly.The helper functions properly implement keypair derivation from both mnemonics and base58-encoded private keys with appropriate error handling.
54-61: Wallet initialization looks good.Properly handles keypair initialization from either private key or mnemonic with appropriate error handling.
63-68: API endpoint selection is correct.The code properly selects the appropriate Solana API endpoint based on the network parameter.
149-164: Command definition looks good.The CLI command is well-defined with appropriate required and optional parameters. The usage of Option with choices for network is a good practice.
51-52:❌ Incorrect review comment
Consider environment-specific IDL selection.
The code selects the Gateway IDL based on network, but uses the same IDL for both mainnet and devnet. Ensure this approach is correct as mainnet may require a stable IDL version.
🏁 Script executed:
#!/bin/bash # Check if there are references to different IDL usage for mainnet vs devnet rg -A 5 -B 5 "mainnet.*IDL|devnet.*IDL" --type tsLength of output: 744
🏁 Script executed:
#!/bin/bash # Locate where GATEWAY_DEV_IDL and GATEWAY_PROD_IDL are defined to verify if a separate devnet IDL exists rg -n "GATEWAY_DEV_IDL" --type ts rg -n "GATEWAY_PROD_IDL" --type tsLength of output: 565
No separate devnet IDL exists
The@zetachain/protocol-contracts-solanapackage only provides two IDLs—
dev/idl/gateway.json(used forlocalnet)prod/idl/gateway.json(used for everything else, including both devnet and mainnet)As such, using
GATEWAY_PROD_IDLfor both devnet and mainnet is intentional and correct. No changes are required unless you plan to introduce a distinct devnet IDL.• File: packages/commands/src/solana/deposit.ts
Lines: 51–52Likely an incorrect or invalid review comment.
|
Modified |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
typechain-types/factories/contracts/EthZetaMock.sol/ZetaEthMock__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/SwapHelpers.sol/SwapLibrary__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/TestZRC20__factory.tsis excluded by!typechain-types/**typechain-types/factories/contracts/shared/MockZRC20__factory.tsis excluded by!typechain-types/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
package.json(1 hunks)packages/client/src/solanaDeposit.ts(2 hunks)packages/client/src/solanaDepositAndCall.ts(2 hunks)packages/commands/src/solana/deposit.ts(1 hunks)packages/commands/src/solana/index.ts(1 hunks)packages/tasks/src/solanaDeposit.ts(1 hunks)packages/tasks/src/solanaDepositAndCall.ts(1 hunks)utils/accounts.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/tasks/src/solanaDeposit.ts
- packages/tasks/src/solanaDepositAndCall.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- package.json
- packages/client/src/solanaDeposit.ts
- packages/commands/src/solana/index.ts
- packages/client/src/solanaDepositAndCall.ts
- packages/commands/src/solana/deposit.ts
🔇 Additional comments (3)
utils/accounts.ts (3)
5-5: Added bs58 library for base58 encoding/decodingGood choice to add the bs58 library to handle base58 encoding/decoding for Solana private keys. This is standard practice when working with Solana keypairs.
83-105: Enhanced flexibility for handling Solana private keysThe improved private key handling now supports both base58 and hex formats, which aligns well with the new Solana deposit command requirements. The error handling is comprehensive with helpful error messages.
108-108:❓ Verification inconclusive
Potential backward compatibility issue
Changing the private key encoding from hex to base58 might break backward compatibility with code that expects the private key to be in hex format. Consider verifying that all consuming code can handle this change.
🏁 Script executed:
#!/bin/bash # Search for uses of SolanaAccountData that might be affected by the encoding change echo "Searching for places where SolanaAccountData private keys are used..." rg -A 5 "SolanaAccountData" --type typescript echo "Searching for places where Solana private keys might be accessed..." rg -A 5 "\.privateKey.*solana" --type typescriptLength of output: 460
🏁 Script executed:
#!/bin/bash # Search for references without relying on --type filter echo "Searching for uses of SolanaAccountData in .ts files..." rg -A5 "SolanaAccountData" -g '*.ts' echo "Searching for privateKey fields in .ts files..." rg -A5 "\.privateKey" -g '*.ts'Length of output: 7950
Verify backward compatibility of Solana privateKey encoding
Changing the Solana private key output from hex to base58 will break any code or persisted data that still expects a hex string. Please confirm that all consumers have been updated (or that legacy hex keys are still supported), including:
• utils/accounts.ts – createSolanaAccount now does
privateKey: bs58.encode(keypair.secretKey)• utils/addressResolver.ts – code loading SolanaAccountData from disk
• packages/commands/src/solana/deposit.ts – keypairFromPrivateKey(...) must decode base58
• Any other CLI commands or helpers that read account.privateKey (e.g. in bitcoin, evm, sui)
• tests or migrations for existing user‐stored keysIf any consumers still expect hex, either restore hex support or add a migration/dual-format parser.
| @@ -15,7 +15,7 @@ jobs: | |||
| - name: Setup Node.js | |||
| uses: actions/setup-node@v3 | |||
| with: | |||
| node-version: "18" | |||
| node-version: "21" | |||
There was a problem hiding this comment.
Ideally, we should address these types of changes in their own separate PR.
There was a problem hiding this comment.
I agree, but the test were failing because of the node version. So either we update node in this PR or merge with failing tests.
| @@ -1,5 +1,5 @@ | |||
| import { Wallet } from "@coral-xyz/anchor"; | |||
| import { Keypair } from "@solana/web3.js"; | |||
| import { Keypair } from "@coral-xyz/anchor/node_modules/@solana/web3.js"; | |||
| .addOption( | ||
| new Option("--network <network>", "Solana network").choices(networks) | ||
| ) | ||
| .action(main); |
There was a problem hiding this comment.
Let's please use a Zod schema to validate params here.
| } else if (options.mnemonic) { | ||
| keypair = await keypairFromMnemonic(options.mnemonic); | ||
| } else { | ||
| throw new Error("Either privateKey or mnemonic must be provided"); |
There was a problem hiding this comment.
This shouldn't be necessary if we validate params with a solid Zod schema.
There was a problem hiding this comment.
Right now we're duplicating logic: optionality, default values, conflicts are handled both in commander and zod.
There was a problem hiding this comment.
I can see advantages of zod for exported functions, but less so for commands that already use declarative syntax to do validation.
For example,
.refine((data) => !(data.mnemonic && data.privateKey), {
message: "Only one of mnemonic or privateKey can be provided, not both",
});And:
.addOption(
new Option("--name <name>", "Account name")
.conflicts(["private-key"])
)
.addOption(
new Option("--private-key <key>", "Private key for signing transactions")
.conflicts(["name"])
)We don't need both.
There was a problem hiding this comment.
Maybe we do types in zod like .string(), but default values and validation in commander.
There was a problem hiding this comment.
Default values and enum values should definitely be in commander, because they show up in --help.
There was a problem hiding this comment.
Added zod validation: a4f049f
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
utils/solana.commands.helpers.ts (3)
5-14: Consider adding stricter validation for field formatsThe schema efficiently defines the required fields and their types, but could benefit from more specific format validation:
amountshould validate that it's a valid number stringrecipientshould validate as a valid Solana address (base58 encoded public key)mintshould validate as a valid Solana address when providednetworkshould restrict to supported networks (devnet, localnet, mainnet)export const solanaDepositOptionsSchema = z .object({ - amount: z.string(), + amount: z.string().refine((val) => !isNaN(parseFloat(val)), { + message: "Amount must be a valid number", + }), mint: z.string().optional(), mnemonic: z.string().optional(), - network: z.string(), + network: z.enum(["devnet", "localnet", "mainnet"]), privateKey: z.string().optional(), - recipient: z.string(), + recipient: z.string().regex(/^[1-9A-HJ-NP-Za-km-z]{32,44}$/, { + message: "Recipient must be a valid Solana address", + }), tokenProgram: z.string().default(SOLANA_TOKEN_PROGRAM), })
15-17: Consider enforcing at least one authentication methodThe refinement correctly ensures that both authentication methods aren't provided simultaneously, but doesn't enforce that at least one is provided.
.refine((data) => !(data.mnemonic && data.privateKey), { message: "Only one of mnemonic or privateKey can be provided, not both", }); + .refine((data) => !!(data.mnemonic || data.privateKey), { + message: "Either mnemonic or privateKey must be provided", + });
1-18: Add JSDoc comments for better documentationConsider adding JSDoc comments to explain the purpose of the schema and document each field's requirements. This would improve developer experience and make the code more maintainable.
+/** + * Schema for validating Solana deposit command options. + * Used to ensure all required fields are present and correctly formatted. + */ export const solanaDepositOptionsSchema = z .object({ + /** + * Amount to deposit, must be a valid number as string + */ amount: z.string(), + /** + * Optional SPL token mint address for token deposits. + * Required when depositing SPL tokens, not used for SOL deposits. + */ mint: z.string().optional(), + /** + * Optional BIP39 mnemonic phrase for wallet derivation. + * Mutually exclusive with privateKey. + */ mnemonic: z.string().optional(), + /** + * Solana network to use (devnet, localnet, mainnet) + */ network: z.string(), + /** + * Optional base58-encoded private key for wallet. + * Mutually exclusive with mnemonic. + */ privateKey: z.string().optional(), + /** + * Destination Solana address for the deposit + */ recipient: z.string(), + /** + * Token program address, defaults to standard Solana token program + */ tokenProgram: z.string().default(SOLANA_TOKEN_PROGRAM), })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/commands/src/solana/deposit.ts(1 hunks)utils/solana.commands.helpers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/commands/src/solana/deposit.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
utils/solana.commands.helpers.ts (1)
types/shared.constants.ts (1)
SOLANA_TOKEN_PROGRAM(2-3)
🔇 Additional comments (1)
utils/solana.commands.helpers.ts (1)
1-4: Nice clean imports!The imports are well-organized with clear separation between external libraries (zod) and internal modules.
| export const keypairFromMnemonic = async ( | ||
| mnemonic: string | ||
| ): Promise<anchor.web3.Keypair> => { | ||
| const seed = await bip39.mnemonicToSeed(mnemonic); | ||
| const seedSlice = new Uint8Array(seed).slice(0, 32); | ||
| return anchor.web3.Keypair.fromSeed(seedSlice); | ||
| }; | ||
|
|
||
| export const keypairFromPrivateKey = ( | ||
| privateKey: string | ||
| ): anchor.web3.Keypair => { | ||
| try { | ||
| const decodedKey = bs58.decode(privateKey); | ||
| return anchor.web3.Keypair.fromSecretKey(decodedKey); | ||
| } catch (error) { | ||
| throw new Error( | ||
| "Invalid private key format. Expected base58-encoded private key." | ||
| ); | ||
| } | ||
| }; |
There was a problem hiding this comment.
These seem useful, we should probably extract them to a shared folder so they can be reused.
| .addOption( | ||
| new Option("--mnemonic <mnemonic>", "Mnemonic").conflicts(["private-key"]) | ||
| ) | ||
| .addOption( | ||
| new Option( | ||
| "--private-key <privateKey>", | ||
| "Private key in base58 format" | ||
| ).conflicts(["mnemonic"]) | ||
| ) |
There was a problem hiding this comment.
Shouldn't we have the name argument and allow users to run the command with their saved account?
| "--private-key <privateKey>", | ||
| "Private key in base58 format" |
There was a problem hiding this comment.
The result of the accounts show command returns a pk in hex format, but this command requires a base58 pk, while it surely is a simple conversion it is still inconvenient.
There was a problem hiding this comment.
Not sure that's the case. Here's what accounts show returns (this is a throwaway account):
{
"address": "yZKkC2e4Ym49nSEtHFrEq7bX7KB2o5sSceNWZvscEzx",
"privateKey": "3UNF7UcazWuZMyvvao4u8Z73MaZVK...7RpLeAWdfCTYXvz5dVpF2BVH63jHkGU",
"privateKeyEncoding": "base58",
"privateKeyScheme": "ed25519",
"publicKey": "yZKkC2e4Ym49nSEtHFrEq7bX7KB2o5sSceNWZvscEzx"
}
Deposit expects base58, because that's what Phantom exports by default.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/commands/src/solana/deposit.ts(1 hunks)utils/accounts.ts(2 hunks)utils/solana.commands.helpers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- utils/accounts.ts
- packages/commands/src/solana/deposit.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
utils/solana.commands.helpers.ts (1)
types/shared.constants.ts (1)
SOLANA_TOKEN_PROGRAM(2-3)
🔇 Additional comments (2)
utils/solana.commands.helpers.ts (2)
1-6: LGTM! Clean imports with appropriate dependencies.The imports are well-organized and all dependencies are used appropriately for Solana operations, mnemonic handling, base58 encoding, and schema validation.
30-41: LGTM! Robust private key handling with proper error handling.The function correctly:
- Decodes base58-encoded private keys (standard for Solana)
- Uses appropriate Solana keypair creation method
- Provides clear error messaging for invalid formats
- Handles exceptions gracefully
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
utils/solana.commands.helpers.ts (2)
12-24: Add validation to ensure at least one authentication method is provided.The schema correctly enforces mutual exclusivity between
mnemonicandprivateKey, but it doesn't ensure that at least one authentication method is provided. This could lead to runtime errors when trying to create a keypair.Apply this diff to ensure at least one authentication method is provided:
.refine((data) => !(data.mnemonic && data.privateKey), { message: "Only one of mnemonic or privateKey can be provided, not both", }) + .refine((data) => data.mnemonic || data.privateKey, { + message: "Either mnemonic or privateKey must be provided", + });
26-32: Add error handling for invalid mnemonic phrases.The function correctly implements BIP39 mnemonic to seed conversion and proper seed slicing for Solana keypairs. However, it lacks explicit error handling for invalid mnemonic phrases.
Apply this diff to add proper error handling:
export const keypairFromMnemonic = async ( mnemonic: string ): Promise<anchor.web3.Keypair> => { + if (!bip39.validateMnemonic(mnemonic)) { + throw new Error("Invalid mnemonic phrase"); + } const seed = await bip39.mnemonicToSeed(mnemonic); const seedSlice = new Uint8Array(seed).slice(0, 32); return anchor.web3.Keypair.fromSeed(seedSlice); };
🧹 Nitpick comments (1)
packages/commands/src/solana/deposit.ts (1)
30-35: Potential undefined keypair issue.The code doesn't handle the case where neither
privateKeynormnemonicis provided, which could lead tokeypairbeing undefined. While the schema validation should prevent this, adding explicit error handling would improve robustness.Apply this diff to add explicit error handling:
let keypair: anchor.web3.Keypair; if (options.privateKey) { keypair = keypairFromPrivateKey(options.privateKey); } else if (options.mnemonic) { keypair = await keypairFromMnemonic(options.mnemonic); + } else { + throw new Error("Either mnemonic or privateKey must be provided"); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/commands/src/solana/deposit.ts(1 hunks)utils/solana.commands.helpers.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/commands/src/solana/deposit.ts (1)
Learnt from: fadeev
PR: zeta-chain/toolkit#317
File: packages/commands/src/solana/deposit.ts:140-140
Timestamp: 2025-05-21T08:42:41.583Z
Learning: The native SOL deposit method in the Solana Gateway program works correctly with an empty accounts object, as the program handles the necessary accounts internally.
🧬 Code Graph Analysis (2)
utils/solana.commands.helpers.ts (1)
types/shared.constants.ts (2)
SOLANA_TOKEN_PROGRAM(2-3)SOLANA_NETWORKS(4-4)
packages/commands/src/solana/deposit.ts (4)
utils/solana.commands.helpers.ts (4)
solanaDepositOptionsSchema(12-24)keypairFromPrivateKey(34-45)keypairFromMnemonic(26-32)createSolanaCommandWithCommonOptions(47-64)utils/handleError.ts (1)
handleError(31-45)types/shared.constants.ts (1)
SOLANA_TOKEN_PROGRAM(2-3)utils/validateAndParseSchema.ts (1)
validateAndParseSchema(28-59)
🪛 GitHub Check: build
packages/commands/src/solana/deposit.ts
[failure] 1-1:
Run autofix to sort these imports!
🪛 ESLint
packages/commands/src/solana/deposit.ts
[error] 1-21: Run autofix to sort these imports!
(simple-import-sort/imports)
🪛 GitHub Actions: Lint
packages/commands/src/solana/deposit.ts
[error] 1-1: ESLint: Run autofix to sort these imports! (simple-import-sort/imports)
🔇 Additional comments (8)
utils/solana.commands.helpers.ts (2)
34-45: Excellent error handling for private key validation.The
keypairFromPrivateKeyfunction properly handles base58 decoding errors and provides clear error messages. The implementation correctly uses try-catch to handle invalid private key formats.
47-64: Well-designed command helper function.The
createSolanaCommandWithCommonOptionsfunction provides a clean abstraction for creating Solana commands with common options. The use ofconflicts()ensures mutual exclusivity between mnemonic and private-key options, and the choices restriction for networks is appropriate.packages/commands/src/solana/deposit.ts (6)
46-46: Non-null assertion should be safe here.The non-null assertion (
keypair!) is acceptable here since the schema validation ensures that a keypair will be created from either mnemonic or privateKey. However, the previous suggestion to add explicit error handling would eliminate the need for this assertion.
61-88: Excellent decimal handling and balance validation.The implementation correctly fetches the actual decimal places from the mint info for SPL tokens and validates the user's token balance before attempting the transaction. This addresses previous review feedback and ensures accurate token handling.
127-136: Good SOL balance validation.The SOL balance check prevents failed transactions by ensuring sufficient funds are available before attempting the deposit. The conversion to lamports and balance comparison logic is correct.
143-143: Empty accounts object is acceptable for native SOL deposits.Based on the retrieved learnings, the native SOL deposit method in the Solana Gateway program works correctly with an empty accounts object, as the program handles the necessary accounts internally.
147-154: Proper error handling with context.The error handling uses the shared
handleErrorutility to provide context and properly exits the process on failure. This ensures users receive clear error messages when transactions fail.
157-175: Well-structured command definition.The command is properly structured using the shared helper function and includes appropriate schema validation. The use of
validateAndParseSchemawithexitOnError: trueensures proper validation and user-friendly error messages.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/commands/src/solana/deposit.ts (1)
1-23: Import sorting issue flagged by ESLint.The imports need to be reordered according to the project's ESLint configuration.
🧹 Nitpick comments (1)
packages/commands/src/solana/deposit.ts (1)
32-45: Improve type safety by restructuring keypair initialization.The current code uses non-null assertions throughout the file, which could be avoided with better TypeScript flow analysis.
Consider restructuring the keypair creation to avoid non-null assertions:
-let keypair: anchor.web3.Keypair; -if (options.privateKey) { - keypair = keypairFromPrivateKey(options.privateKey); -} else if (options.mnemonic) { - keypair = await keypairFromMnemonic(options.mnemonic); -} else if (options.name) { - const privateKey = getAccountData<SolanaAccountData>( - "solana", - options.name - )?.privateKey; - keypair = keypairFromPrivateKey(privateKey!); -} else { - throw new Error("No account provided"); -} +const keypair = await (async (): Promise<anchor.web3.Keypair> => { + if (options.privateKey) { + return keypairFromPrivateKey(options.privateKey); + } else if (options.mnemonic) { + return await keypairFromMnemonic(options.mnemonic); + } else if (options.name) { + const accountData = getAccountData<SolanaAccountData>("solana", options.name); + if (!accountData?.privateKey) { + throw new Error(`No private key found for account: ${options.name}`); + } + return keypairFromPrivateKey(accountData.privateKey); + } else { + throw new Error("No account provided"); + } +})();This eliminates the need for non-null assertions later in the code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/commands/src/solana/deposit.ts(1 hunks)types/accounts.types.ts(1 hunks)utils/solana.commands.helpers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- utils/solana.commands.helpers.ts
🧰 Additional context used
🧠 Learnings (1)
packages/commands/src/solana/deposit.ts (1)
Learnt from: fadeev
PR: zeta-chain/toolkit#317
File: packages/commands/src/solana/deposit.ts:140-140
Timestamp: 2025-05-21T08:42:41.583Z
Learning: The native SOL deposit method in the Solana Gateway program works correctly with an empty accounts object, as the program handles the necessary accounts internally.
🔇 Additional comments (3)
types/accounts.types.ts (1)
31-31: LGTM! Schema update aligns with private key standardization.The renaming from
secretKeytoprivateKeyimproves consistency across the codebase and aligns with the updated private key handling that now supports base58-encoded strings.packages/commands/src/solana/deposit.ts (2)
69-165: Well-implemented deposit logic with proper error handling.The implementation correctly handles both SOL and SPL token deposits with the following improvements from past reviews:
- ✅ Dynamic decimal fetching for SPL tokens
- ✅ Balance checks for both SOL and SPL tokens
- ✅ Proper error handling with try-catch
- ✅ Schema validation using Zod
The empty accounts object for native SOL deposits is intentional as confirmed by previous feedback.
167-185: Command definition follows established patterns.The command setup using
createSolanaCommandWithCommonOptionsand schema validation is well-structured and consistent with the project's patterns.
Deposit SOL
✅ Devnet
https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/3Tw2S8HPbFAN7xtMase8oLa23d1f9etacTfhuREbGnccuSHTkREbtM9sB3H2H5pYbEhpWMs74wA3QS5wxhSnGsSM
✅ Localnet
Deposit SPL
✅ Devnet
https://zetachain-athens.blockpi.network/lcd/v1/public/zeta-chain/crosschain/inboundHashToCctxData/5LjZVoeCScPqmVBkPGrY4LWX3ukDE2f8z28vjbEbDwJPtmmUPKG1o6D8T3uc61h78NKisxQyABhYMmnVi2secydr
✅ Localnet
Right now it's a bit confusing, because by default localnet airdrops SPL tokens only to the account defined in
~/.config/solana/id.json, and not to the address associated with the default mnemonic below.Summary by CodeRabbit
New Features
Chores
Improvements